Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

{Keyvault} Fix #22540: Create keyvault with service principal #22543

Merged
merged 1 commit into from
May 20, 2022
Merged

{Keyvault} Fix #22540: Create keyvault with service principal #22543

merged 1 commit into from
May 20, 2022

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented May 20, 2022 •
edited
Loading

Related command
az keyvault create

Description

#22203 changed msrestazure.azure_exceptions.CloudError to azure.cli.command_modules.role.GraphError in azure.cli.command_modules.keyvault.custom._get_current_user_object_id.

Some background: azure-graphrbac Python SDK raises inconsistent exceptions for Graph errors:

DomainsOperations.get raises CloudError:

https://github.com/Azure/azure-sdk-for-python/blob/dfa109c36598d93aaf74238feded516352c20f4c/sdk/graphrbac/azure-graphrbac/azure/graphrbac/operations/domains_operations.py#L150-L153

        if response.status_code not in [200]:
            exp = CloudError(response)
            exp.request_id = response.headers.get('x-ms-request-id')
            raise exp

SignedInUserOperations.get raises GraphErrorException:

https://github.com/Azure/azure-sdk-for-python/blob/dfa109c36598d93aaf74238feded516352c20f4c/sdk/graphrbac/azure-graphrbac/azure/graphrbac/operations/signed_in_user_operations.py#L79-L80

        if response.status_code not in [200]:
            raise models.GraphErrorException(self._deserialize, response)

Since SignedInUserOperations.get doesn't raise CloudError, this except CloudError will never be hit:

azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py

Lines 312 to 317 in 1451bd5

try:
current_user = graph_client.signed_in_user.get()
if current_user and current_user.object_id: # pylint:disable=no-member
return current_user.object_id # pylint:disable=no-member
except CloudError:
pass

Therefore, this except CloudError clause has no effect and should NOT be replaced by GraphError. Doing so will make GraphError be intercepted and the outer except GraphError have no effect:

azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py

Lines 747 to 750 in cac3f06

try:
object_id = _get_current_user_object_id(graph_client)
except GraphError:
object_id = _get_object_id(graph_client, subscription=subscription)

This PR lets _get_current_user_object_id raise GraphError so that it can be caught by outer except GraphError.

Testing Guide

az ad sp create-for-rbac --role Owner --scopes /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590 --verbose

# Copy the login command from log and run:
az login --service-principal --username 558c5061-b77b-4dfa-8118-168068c9be4a --password xxx --tenant 54826b22-38d6-4fb2-bad9-b7b93a3e9c5a

az group create -n my-kv-testrg -l eastasia
az keyvault create -n my-kv-test -g my-kv-testrg

@ghost ghost added the Auto-Assign Auto assign by bot label May 20, 2022
@ghost ghost requested a review from yonzhan May 20, 2022 09:51
@ghost ghost assigned evelyn-ys May 20, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone May 20, 2022
@ghost ghost added the KeyVault az keyvault label May 20, 2022
jiasli
jiasli commented May 20, 2022
View reviewed changes
src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Comment on lines -311 to -313
current_user = graph_client.signed_in_user_get()
if current_user and current_user.get('id', None): # pylint:disable=no-member
return current_user['id'] # pylint:disable=no-member
Copy link
Member Author

@jiasli jiasli May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did leave a comment for this weird behavior of checking the existence of id (#22203 (comment)).

graph_client.signed_in_user_get will ALWAYS return a user object which MUST have id property UNLESS GraphError is raised.

Takeaways:

  1. Pylint error is a sign of abnormality and should NOT be disabled brutally.
  2. For anything unusual like current_user.get('id', None) or except CloudError, comments MUST be left to explain its behavior. Otherwise, it should not be there.

@jiasli jiasli requested a review from FumingZhang May 20, 2022 10:16
jiasli
jiasli commented May 20, 2022
View reviewed changes
src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Comment on lines -314 to -315
except GraphError:
pass
Copy link
Member Author

@jiasli jiasli May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got tripped by some code written almost 6 years ago! (cb0faa8)

@yonzhan
Copy link
Collaborator

yonzhan commented May 20, 2022

Good catch !

@jiasli jiasli merged commit 23b944a into Azure:dev May 20, 2022
@jiasli jiasli deleted the keyvault-sp branch May 20, 2022 10:49
@jiasli
Copy link
Member Author

jiasli commented May 20, 2022

I have confirmed the new RPM has the correct code:

> docker run -it --rm mcr.microsoft.com/cbl-mariner/base/core:2.0 bash

# tdnf install https://azurecliprod.blob.core.windows.net/archive/20220520.4/rpm-mariner2.0/signed/azure-cli-2.37.0-1.x86_64.rpm
# tdnf install sed
# cat /lib64/az/lib/python3.9/site-packages/azure/cli/command_modules/keyvault/custom.py | sed -n '307,309p'
def _get_current_user_object_id(graph_client):
    current_user = graph_client.signed_in_user_get()
    return current_user['id']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@calvinhzy calvinhzy calvinhzy approved these changes

@yonzhan yonzhan Awaiting requested review from yonzhan

@FumingZhang FumingZhang Awaiting requested review from FumingZhang

Assignees

@evelyn-ys evelyn-ys

Labels
Auto-Assign Auto assign by bot KeyVault az keyvault Microsoft Graph
Projects
None yet
Milestone
May 2022 (2022-05-24) - For Build
Development

Successfully merging this pull request may close these issues.

Could not create keyvault with service principal after graph module migration
4 participants
@jiasli @yonzhan @evelyn-ys @calvinhzy